[fix](streaming-job) Optimize snapshot offset persistence - #66238
Open
JNSimba wants to merge 2 commits into
Open
[fix](streaming-job) Optimize snapshot offset persistence#66238JNSimba wants to merge 2 commits into
JNSimba wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29504 ms |
Contributor
TPC-DS: Total hot run time: 178444 ms |
Contributor
ClickBench: Total hot run time: 25.17 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed the full authoritative change set. I found one merge-blocking regression-test authoring issue; I did not find a PR-introduced production correctness defect.
Part 1.3 checkpoint conclusions:
- Goal and proof: The change reduces snapshot-offset EditLog volume, compacts obsolete snapshot recovery state after the binlog transition, durably records natural
FINISHEDstate, and supplies a 40960 default snapshot split size. The production implementation accomplishes those goals. Focused FE tests exercise persistence timing, binlog immediacy, state compaction, replayed terminal fields/callback cleanup, and snapshot-only restart, but the new regression is not authored in the repository's required result-test format and the exact default/override split-size values are not directly asserted. - Scope: The production changes are small and focused on the job/provider lifecycle and the two MySQL/PostgreSQL reader construction paths.
- Concurrency: Job state is serialized by the existing job write lock, while provider split/progress state remains protected by
splitsLock. The new transient persistence timestamp is only used inside the job-locked commit path. Async split enumeration keeps its existing publish recheck, no provider RPC was moved undersplitsLock, no new nested lock order was introduced, and I found no deadlock or heavyweight-operation regression. - Lifecycle: Normal execution, recovery scheduling, EditLog replay, checkpoint-image restore, transaction callbacks, and finished-job cleanup were traced. A crash before the terminal job log resumes from conservative durable offset state and rediscovers the end; a crash after it replays status and finish time. Callback registration/removal remains balanced. There are no C++ static-initialization concerns in this FE/Java-only change.
- Configuration: The persistence interval is mutable and read on every eligible snapshot commit, so changes take effect without restart; a new master's transient zero timestamp forces an immediate first journal. Both reader paths apply 40960 only when
snapshot_split_sizeis absent, while validated explicit values still win. - Compatibility: No RPC, function-symbol, or storage-format contract changes were introduced. The timer is transient, old serialized jobs safely use its zero default, and existing provider JSON fields remain readable across replay/rolling upgrade.
- Parallel paths: Base and TVF providers, normal and recovery completion, cloud and non-cloud scheduling, MySQL and PostgreSQL readers, and all startup modes were checked. The corresponding paths are updated consistently.
- Conditional checks: The non-empty binlog-offset invariant is enforced before snapshot-state cleanup in both providers and is the direct precondition for entering durable binlog state. Broader source-specific validation of malformed non-empty ALTER offsets is pre-existing behavior, not a regression introduced by this cleanup.
- Test coverage: The new tests exercise the core timing/replay/cleanup cases and a snapshot-only restart. The regression must be revised to hardcode its ordinary single-table name and expose deterministic status/count checks through named
qt/order_qtoutput rather than raw Groovy assertions. A separate narrow gap is the lack of a direct reader assertion for absent property -> 40960 and explicit property -> exact configured value (including both source types); I treated that as nonblocking because the implementation is a direct validatedgetOrDefaultpath. Per the review environment's constraints, I performed static review only and did not run builds or tests. - Test results: The deterministic expectations are consistent with the traced behavior, but the suite uses
def tableNameand rawasserts for stable query results, contrary to the mandatory regression-test rules, and the matching runner-generated.outfile is absent. That result artifact must be generated through the prescribed regression runner, not handwritten. - Observability: Existing job/offset logs retain identifiers and error context sufficient for recovery diagnosis. The new rate-limited path removes repeated persistence work and does not need a per-commit INFO log or a new critical-path metric.
- Transactions, persistence, and data writes: Offset update/replay ordering, EditLog coverage, image restore, and transaction callback behavior remain consistent. A failover during a suppressed snapshot interval can move the durable resume point backward and reprocess work, but cannot advance it past unjournaled data or skip a source range; that is consistent with the existing at-least-once/unique-key target path.
- FE-BE variables: No new variable or protocol field needs propagation between FE and BE.
- Performance and other issues: The change removes redundant snapshot EditLog writes and uses a larger default split size to reduce split overhead. Remaining per-commit serialization is pre-existing; I found no new CPU, memory, allocation, or algorithmic issue and no other blocking defect.
There was no additional review-focus guidance, so this conclusion covers the whole PR. Please fix the regression-test authoring issue before merge.
| SELECT COUNT(*), COUNT(DISTINCT id) | ||
| FROM ${currentDb}.${tableName} | ||
| """ | ||
| assert rowsBeforeRestart.size() == 1 |
Contributor
There was a problem hiding this comment.
Please bring this new regression into compliance with the repository's mandatory test rules. For a single ordinary table, hardcode snapshot_finished_restart_fe instead of def tableName; and record these deterministic pre/post-restart count/status checks through named qt/order_qt queries with the runner-generated .out, rather than raw Groovy asserts. The Awaitility readiness and dynamic job-ID checks can remain operational assertions.
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
Snapshot tasks persisted the complete offset provider after every split commit, producing frequent edit logs while the snapshot state kept growing. The previous default snapshot split size also produced too many small snapshot tasks. Natural terminal transitions needed explicit persistence so finished jobs replay with their terminal metadata.
This change:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)